Skip to content

Fix Windows network detection leak and add periodic leak reports - #1536

Open
bmehta001 wants to merge 45 commits into
microsoft:mainfrom
bmehta001:bhamehta/periodic-memory-leak-analysis
Open

bmehta001 wants to merge 45 commits into
microsoft:mainfrom
bmehta001:bhamehta/periodic-memory-leak-analysis

Conversation

@bmehta001

@bmehta001 bmehta001 commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace desktop PublicNetworkListManager use with Windows.Networking.Connectivity, preserving metered-network cost updates without loading the leaking netprofm.dll
  • run Dr. Memory analysis on Windows and Linux after relevant changes reach main, with manual dispatch available on demand
  • analyze unit tests, functional tests, and SampleCppMini with pinned, SHA-256-verified tooling
  • publish leak counts to the job summary and retain raw reports for 90 days
  • keep the current leak baseline non-gating while failing explicitly on instrumentation/target errors
  • fail Windows leak analysis if netprofm.dll is loaded again

Validation

  • Visual Studio 2026 x64 Debug build of UnitTests and SampleCppMini
  • focused Windows network/configuration tests: 31 passed
  • GCC/WSL syntax check of SampleCppMini
  • actionlint 1.7.12 and PowerShell parser validation
  • hosted Dr. Memory runs on Windows and Linux, including the netprofm.dll regression gate

Known instrumentation exclusion

  • BasicFuncTests.killSwitchWorks remains covered by normal CI but is excluded under Dr. Memory because instrumentation changes its exact asynchronous drop count (400 observed versus 100 expected).
  • OfflineStorageTests_SQLite.StoreThousandEventsTakesLessThanASecond remains covered by normal CI but is excluded under Dr. Memory because instrumentation invalidates its one-second wall-clock performance threshold (1.384 seconds observed).

Closes #634

Related external evidence: microsoft/onnxruntime-genai#2590

Track Windows and Linux leak counts without making the known baseline block unrelated changes. Pin and verify Dr. Memory, retain raw reports, and publish per-scenario summaries for unit tests, functional tests, and SampleCppMini.

Files changed:
- .github/workflows/memory-leak-analysis.yml
- .github/scripts/run-drmemory.ps1

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Run the expensive analysis only when its workflow or helper changes, so this PR and future maintenance updates exercise both hosted platforms before merge.

Files changed:
- .github/workflows/memory-leak-analysis.yml

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
@bmehta001
bmehta001 requested a review from a team as a code owner September 18, 2026 01:07
Build Linux targets without entering the package deployment path, and ignore Dr. Memory's incomplete Windows bootstrap report while retaining it in the raw artifact.

Files changed:
- .github/scripts/run-drmemory.ps1
- .github/workflows/memory-leak-analysis.yml

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Install the Linux curl development dependency, avoid the unrelated installed-package target regression when compiling the sample, and exclude the one functional assertion whose exact asynchronous drop count changes under instrumentation.

Files changed:
- .github/workflows/memory-leak-analysis.yml

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Recognize Dr. Memory's clean-report marker and disambiguate SampleCppMini's signed 64-bit EventProperty construction so the same sample compiles under GCC and MSVC.

Files changed:
- .github/scripts/run-drmemory.ps1
- examples/cpp/SampleCppMini/main.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Use Windows.Networking.Connectivity for both cost queries and change notifications so network detection preserves behavior without instantiating PublicNetworkListManager or loading netprofm.dll. Fail periodic leak analysis if netprofm returns.

Files changed:
- lib/pal/desktop/NetworkDetector.cpp
- lib/pal/desktop/NetworkDetector.hpp
- .github/workflows/memory-leak-analysis.yml

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
@bmehta001 bmehta001 changed the title Add periodic Windows and Linux memory leak reports Fix Windows network detection leak and add periodic leak reports Sep 18, 2026
bmehta001 and others added 4 commits September 18, 2026 03:16
Use scoped objects for temporary event and buffer allocations, and destroy the log-session provider during fixture teardown so leak reports represent SDK behavior rather than test fixture ownership.

Files changed:
tests/unittests/AnnexKTests.cpp
tests/unittests/LogSessionDataDBTests.cpp
tests/unittests/TransmissionPolicyManagerTests.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Exercise the real Windows detector so CI proves that WinRT status registration starts, network cost remains valid, shutdown completes, and netprofm.dll is not loaded.

Files changed:
tests/unittests/NetworkDetectorTests.cpp
tests/unittests/CMakeLists.txt
tests/unittests/UnitTests.vcxproj

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Delete the unused NLM interfaces, connection points, callbacks, maps, compatibility branch, and manual reference counting now that network detection is entirely WinRT-based. This reduces object and binary overhead while keeping ownership with unique_ptr.

Files changed:
docs/building-custom-SKU.md
lib/pal/desktop/NetworkDetector.cpp
lib/pal/desktop/NetworkDetector.hpp
lib/pal/desktop/WindowsDesktopNetworkInformationImpl.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
Avoid spending hosted runner time when main has not changed while preserving manual analysis on demand.

Files changed:
- .github/workflows/memory-leak-analysis.yml

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Network-cost behavior regresses for approaching-limit connections, and the leak-analysis coverage has gaps.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Replaces leaking Windows Network List Manager usage with WinRT connectivity APIs and adds automated Dr. Memory reporting.

Changes:

  • Migrates Windows network-cost detection and adds regression coverage.
  • Adds Windows/Linux leak-analysis workflows with verified tooling and retained reports.
  • Fixes test leaks and portability issues.
File summaries
File Description
tests/unittests/UnitTests.vcxproj Adds network detector tests.
tests/unittests/TransmissionPolicyManagerTests.cpp Uses stack-owned event contexts.
tests/unittests/NetworkDetectorTests.cpp Tests WinRT detector lifecycle.
tests/unittests/LogSessionDataDBTests.cpp Releases the session provider.
tests/unittests/CMakeLists.txt Includes Windows detector tests.
tests/unittests/AnnexKTests.cpp Adds RAII for allocated buffers.
lib/pal/desktop/WindowsDesktopNetworkInformationImpl.cpp Removes obsolete COM reference counting.
lib/pal/desktop/NetworkDetector.hpp Defines the simplified WinRT detector.
lib/pal/desktop/NetworkDetector.cpp Implements WinRT cost monitoring.
examples/cpp/SampleCppMini/main.cpp Makes integer width explicit.
docs/building-custom-SKU.md Documents WinRT network detection.
.github/workflows/memory-leak-analysis.yml Adds cross-platform leak-analysis jobs.
.github/scripts/run-drmemory.ps1 Runs Dr. Memory and summarizes results.
Review details
  • Files reviewed: 13/13 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/memory-leak-analysis.yml Outdated
Comment thread lib/pal/desktop/NetworkDetector.cpp Outdated
Comment thread tests/unittests/NetworkDetectorTests.cpp
Restore approaching-data-limit handling in the WinRT cost mapping, verified in lib/pal/desktop/NetworkDetector.cpp.

Check netprofm.dll while the detector is active, verified in tests/unittests/NetworkDetectorTests.cpp.

Include APITest.C_API_Test in Linux leak analysis after confirming the test passes on Linux, verified in .github/workflows/memory-leak-analysis.yml.

Files changed:
- .github/workflows/memory-leak-analysis.yml
- lib/pal/desktop/NetworkDetector.cpp
- tests/unittests/NetworkDetectorTests.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The asynchronously updated network-cost cache has an unsynchronized read/write data race.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 13/13 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread lib/pal/desktop/NetworkDetector.hpp Outdated
Store network cost and running state atomically so WinRT callbacks cannot race caller reads. Return the cached cost by value instead of exposing a concurrently updated reference.

Verified at:
- lib/pal/desktop/NetworkDetector.cpp
- lib/pal/desktop/NetworkDetector.hpp

Files changed:
- lib/pal/desktop/NetworkDetector.cpp
- lib/pal/desktop/NetworkDetector.hpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The workflow contains an ignored build property, and the new network-cost test does not validate cost mapping or updates.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

.github/workflows/memory-leak-analysis.yml:67

  • MATSDK_USE_WININET is not consumed by any project, props, targets, or source in this repository, so this MSBuild property is silently ignored; the Win32 factory still selects WinInet (lib/http/HttpClientFactory.hpp:27-29). Remove the no-op argument, or wire the intended transport selection into the build before relying on it for this analysis.
    tests/unittests/NetworkDetectorTests.cpp:25
  • This assertion accepts every possible NetworkCost, so an implementation that always returns Unknown still passes and the test does not verify the stated preservation of metered-cost behavior. Add a mocked/injectable WinRT source (or extract the mapping helper) and assert unrestricted, fixed/variable, and roaming/limit mappings, including a status-change refresh.
  • Files reviewed: 13/13 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Remove the ignored MATSDK_USE_WININET build property so leak analysis does not imply a transport selection it never made.

Extract and test WinRT cost mapping for unrestricted, metered, roaming, over-limit, and approaching-limit states, and verify synchronous refresh updates the cache.

Verified at:
- .github/workflows/memory-leak-analysis.yml
- lib/pal/desktop/NetworkDetector.cpp
- tests/unittests/NetworkDetectorTests.cpp

Files changed:
- .github/workflows/memory-leak-analysis.yml
- lib/pal/desktop/NetworkDetector.cpp
- lib/pal/desktop/NetworkDetector.hpp
- tests/unittests/NetworkDetectorTests.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Callback teardown has a potential use-after-free, and the DLL regression gate does not reliably detect module loading.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/memory-leak-analysis.yml:115

  • This does not establish that netprofm.dll was never loaded. Dr. Memory's results.txt only mentions modules that appear in reported errors/stacks, while the module-loading diagnostics are written to global.<pid>.log (and require suitable verbosity); the *.txt filter excludes those logs. Thus a run that loads netprofm.dll without a report involving it passes this advertised regression gate. Enable module-load logging and inspect the global logs, or instrument each target to query its loaded modules directly.
  • Files reviewed: 13/13 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread lib/pal/desktop/NetworkDetector.cpp Outdated
Keep per-subscription callback state alive independently, reject callbacks after shutdown starts, and wait for active callbacks before releasing detector resources.

Inspect Dr. Memory global module logs for netprofm.dll and require logs for every Windows scenario so the regression gate cannot pass vacuously.

Verified at:
- lib/pal/desktop/NetworkDetector.cpp
- .github/workflows/memory-leak-analysis.yml

Files changed:
- .github/workflows/memory-leak-analysis.yml
- lib/pal/desktop/NetworkDetector.cpp
- lib/pal/desktop/NetworkDetector.hpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: df9f344e-e064-404e-ae6f-c0ef455d747c
@bmehta001
bmehta001 requested a balanced review from Copilot September 18, 2026 18:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The detector has a Start/Stop race that can leave its worker running, and the job summary omits tracked totals.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Job summary omits tracked leak totals

.github/​scripts/​run-drmemory.ps1:148

The report collects and baseline-checks TotalPossibleLeaks and TotalReachable, but the job summary omits both values. A regression warning for either metric therefore cannot be interpreted from the promised leak-count summary without downloading the artifact. Include both tracked totals in the table.

Comment thread lib/pal/desktop/NetworkDetector.cpp
Protect all Start and Stop access to the worker thread with a dedicated lifecycle mutex so shutdown cannot miss an unpublished thread. Add a concurrent startup/shutdown regression test.

Show total possible leaks and total reachable allocations in the job summary so every baseline-checked metric is directly interpretable.

Files changed:
- .github/scripts/run-drmemory.ps1
- lib/pal/desktop/NetworkDetector.cpp
- lib/pal/desktop/NetworkDetector.hpp
- tests/unittests/NetworkDetectorTests.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Synchronous network debug callbacks can re-enter teardown and cause the detector thread to join itself or deadlock.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Dispatch coalesced network debug events through an independently owned Windows worker callback. Stop disables future events and waits for external dispatches, but safely returns when called by the active event itself after joining the detector listener.

Add a regression test whose EVT_NET_CHANGED listener synchronously stops the detector; it passes repeatedly without self-join or deadlock.

Files changed:
- lib/pal/desktop/NetworkDetector.cpp
- lib/pal/desktop/NetworkDetector.hpp
- tests/unittests/NetworkDetectorTests.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001

bmehta001 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the latest review summary concern in commit d6bcef3: EVT_NET_CHANGED is now dispatched through an independently owned, coalescing worker callback rather than synchronously on the detector listener. Stop() can therefore join the listener even when invoked reentrantly by an event handler, and the dispatch state prevents any later event. NetworkDetectorTests.NetworkChangeListenerCanStopDetector exercises the exact synchronous teardown path.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The Linux Dr. Memory run omits a required timing-test exclusion and may fail before producing its report.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Exclude SQLite performance test from Linux Dr. Memory run

.github/​workflows/​memory-leak-analysis.yml:197

The Linux unit-test analysis still runs OfflineStorageTests_SQLite.StoreThousandEventsTakesLessThanASecond, even though the PR's known instrumentation exclusions say this test must remain excluded under Dr. Memory because instrumentation violates its one-second threshold. This can fail the Linux job before its leak report is published; pass the same filter used by the Windows invocation.

@bmehta001

Copy link
Copy Markdown
Contributor Author

Copilot's final suppressed note about excluding OfflineStorageTests_SQLite.StoreThousandEventsTakesLessThanASecond on Linux is not actionable. The Windows exclusion was added after the instrumented Windows run took 1.384 seconds. The hosted Linux Dr. Memory job in run 35392664701 executed the unfiltered unit-test binary, completed successfully, and produced its leak report. Keeping the test on Linux preserves coverage; only the platform demonstrated to violate the timing threshold is excluded.

Bring in the latest modules submodule update without rewriting the reviewed PR history.

Files changed:
- lib/modules

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The detector invokes a Windows 8+ API without preserving the repository’s Windows 7 compatibility guard.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread lib/pal/desktop/NetworkDetector.cpp
bmehta001 and others added 11 commits September 22, 2026 03:13
Resolve the Windows workflow conflict while preserving CMake setup and the WinHTTP/WinInet matrix. Set the desktop API floor to Windows 8.1 and remove the pre-8.1 WinHTTP proxy fallback so CI enforces the supported contract.

Files changed: merged upstream main; .github/workflows/test-win-latest.yml; README.md; lib/CMakeLists.txt; lib/http/HttpClient_WinHttp.cpp; Solutions Windows project files; tests/headers/check_public_headers.cmd.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Version and validate the Android CMake/NDK marker so stale setup state cannot hide missing tools. Provision CMake for the Linux no-exceptions job that failed under the runner's 3.31 release.

Raise desktop builds and header gates to the Windows 10 API floor. Remove the Windows 7 runtime probe, hand-defined network-cost GUID, and obsolete warning suppressions in favor of the SDK IID.

Files changed: Android and Linux setup paths, Windows workflows/docs/project definitions, WinHTTP and network detection sources, and the public-header gate.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: fa477318-3257-45cd-8711-d5214b5cb119
Expose the public mbedTLS threading macros to curl so both dependencies compile public context types with identical layouts and avoid an entropy-context overflow.

Files changed: cmake/MatsdkFetchCurl.cmake.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: fa477318-3257-45cd-8711-d5214b5cb119
Added commands to install Android SDK platforms and sources.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Add uuid to the Windows target dependencies so SDK-declared COM GUID symbols resolve for CMake consumers instead of relying on toolchain defaults.

Files changed:
- lib/CMakeLists.txt: propagate the Windows UUID import library through mat.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c72c9f67-f709-4c28-a8fa-809e0aefc14d
…endencies' into bhamehta/standardize-telemetry-dependencies
Disable curl's build-host CA auto-detection and remove generated CA path macros so redistributable Linux binaries rely on the target host's runtime CA selection.

Files changed:
- cmake/MatsdkFetchCurl.cmake: sanitize fetched curl CA defaults and enforce that no build-time path remains.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c72c9f67-f709-4c28-a8fa-809e0aefc14d
Integrate PR microsoft#1537 into PR microsoft#1536 so the dependency and platform updates ship with the leak-analysis work. Resolve the network detector overlap in favor of the leak-safe WinRT lifecycle, narrow the SEH warning suppressions, and consistently enforce the Windows 10 API floor without legacy Windows fallbacks.

Files changed: Windows workflows/projects/docs, CMake dependency setup, Android build setup, WinHTTP transport, and WinRT network detection.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d2b55402-0dec-4ad7-bf0f-d30d92c96171
A live upload to external collector endpoints can fail or stall on CI without indicating a certificate-policy regression. Verify that both Windows transports apply the log configuration directly; retain the separate cold-session HTTPS test for real certificate enforcement.

Files changed: tests/functests/APITest.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d2b55402-0dec-4ad7-bf0f-d30d92c96171

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Periodic memory leak analysis reports

3 participants